Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

reintroduce PyModule constructors #4404

Merged
merged 6 commits into from
Aug 13, 2024
Merged

reintroduce PyModule constructors #4404

merged 6 commits into from
Aug 13, 2024

Conversation

bschoenmaeckers
Copy link
Contributor

No description provided.

Icxolu
Icxolu reviewed Aug 1, 2024
View reviewed changes
Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again! I found a few URLs in the docs that need updating, plus some additional migration opportunities around &CStr that we should discuss about.

guide/src/python-from-rust/calling-existing-code.md Outdated Show resolved Hide resolved
guide/src/python-from-rust/calling-existing-code.md Outdated Show resolved Hide resolved
guide/src/python-from-rust/calling-existing-code.md Outdated Show resolved Hide resolved
src/types/module.rs Outdated Show resolved Hide resolved
src/types/module.rs Show resolved Hide resolved
@Icxolu Icxolu added the CI-skip-changelog Skip checking changelog entry label Aug 1, 2024
@bschoenmaeckers
Copy link
Contributor Author

Sorry for the late reply. Had to find time to do opensource again. I updated the implementation as of your suggestions.

The change on the from_code method is breaking I guess, so should I add a changelog + migration guide entry as well?

@bschoenmaeckers bschoenmaeckers force-pushed the pymodule_new branch 6 times, most recently from 4d34e37 to fecbe59 Compare August 13, 2024 16:20
Icxolu
Icxolu approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finishing this off!

Sorry for the late reply. Had to find time to do opensource again.

No worries

The change on the from_code method is breaking I guess, so should I add a changelog + migration guide entry as well?

Since this is only breaking, once the users already touched the code to migrate from from_code_bound to from_code, I think a changelog entry mentioning this should suffice for now. I suspect that we will have a few more migrations of this kind, so we can add a general migration entry once we collected them all.

src/types/module.rs Outdated Show resolved Hide resolved
@Icxolu Icxolu removed the CI-skip-changelog Skip checking changelog entry label Aug 13, 2024
@bschoenmaeckers
Copy link
Contributor Author

Since this is only breaking, once the users already touched the code to migrate from from_code_bound to from_code, I think a changelog entry mentioning this should suffice for now. I suspect that we will have a few more migrations of this kind, so we can add a general migration entry once we collected them all.

I've added the changelog entry

@Icxolu Icxolu enabled auto-merge August 13, 2024 17:35
@Icxolu
Copy link
Contributor

Icxolu commented Aug 13, 2024

Perfect, thanks!

@Icxolu Icxolu added this pull request to the merge queue Aug 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 13, 2024
@Icxolu Icxolu added this pull request to the merge queue Aug 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 13, 2024
@Icxolu Icxolu added this pull request to the merge queue Aug 13, 2024
Merged via the queue into PyO3:main with commit f869d16 Aug 13, 2024
43 checks passed
@bschoenmaeckers bschoenmaeckers deleted the pymodule_new branch August 14, 2024 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Icxolu Icxolu Icxolu approved these changes

@davidhewitt davidhewitt davidhewitt left review comments

Assignees
No one assigned
Labels
CI-build-full
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bschoenmaeckers @Icxolu @davidhewitt